Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[email protected] #2360

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

publish-to-bcr-bot[bot]
Copy link
Contributor

@bazel-io
Copy link
Member

bazel-io commented Jul 4, 2024

Hello @rickeylev, @f0rmiga, modules you maintain (rules_python) have been updated in this PR. Please review the changes.

@aignas
Copy link
Contributor

aignas commented Jul 4, 2024

Looks like building the wheel for abseil is broken in the example, but that has to do with the something that cmanged in the CI. Could we skip this check and merge as it is?

@Wyverald
Copy link
Member

Wyverald commented Jul 9, 2024

We probably can just force submit this, but looping in @fweikert @meteorcloudy to confirm -- do we need to upgrade the C++ toolchain on the macOS VMs or something?

@meteorcloudy
Copy link
Member

This is not caused by our recent Mac VM migration, those jobs ran on old Mac fleet.

Probably abseil version got upgraded in the new version? Then you'll need

@rickeylev
Copy link
Contributor

rules_python doesn't depend on abseil, though, not directly anyways. Maybe this is coming from something with the proto rules? The failure seems to point to //py_proto_library:message_test as the failure. I'm guessing its trying to compile protoc and failing.

Maybe from this: WARNING: For repository 'rules_proto', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph

@rickeylev
Copy link
Contributor

add --cxxopt=-std=c++14 to build flags

Well, I guess I could add that to our bcr presubmit config, if that fixes it. I don't really like having a different config for BCR vs the actual project, though; it makes last-mile release issues like this more common.

I'm a bit confused, though.

  • We don't have that set in the rules_python CI config, and things work OK there.
  • If we have to add it to this bcr config, doesn't that mean all downstream users of (presumably) rules_proto also have to add it?

The second point seems particularly invasive. Am I missing something?

@rickeylev
Copy link
Contributor

I created #2397 to test out the presubmit.yml changes. Even with the --cxxopt=-std=c++14 added, it still gives the same error: https://buildkite.com/bazel/bcr-presubmit/builds/6616#019099bc-9d89-4381-831e-b8a948231e50

@rickeylev
Copy link
Contributor

Ok, it looks if both --host_cxxopt and --cxxopt are set to the c++14 value, then BCR's CI is happy.

This still feels like an issue on...idk, the bcr or rules_proto side? But we can just add those to our bcr presubmit config in the meantime.

@rickeylev
Copy link
Contributor

I've approved. Can we force-merge this PR? The presubmits from #2397 confirm that the issue is with the presubmit config, and the rules_python code is otherwise fine.

Copy link
Member

@bazel-io bazel-io left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @bazelbuild/bcr-maintainers, all modules in this PR have been approved by their maintainers. Please take a final look to merge this PR.

@meteorcloudy meteorcloudy merged commit a8acfe9 into bazelbuild:main Jul 10, 2024
15 of 17 checks passed
@meteorcloudy
Copy link
Member

@rickeylev Thanks for looking into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants